-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Discover] Skip loading filter if navigating to a saved search without params #10913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…t params Signed-off-by: Joey Liu <[email protected]>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10913 +/- ##
=======================================
Coverage 60.75% 60.75%
=======================================
Files 4533 4533
Lines 122209 122216 +7
Branches 20483 20486 +3
=======================================
+ Hits 74250 74255 +5
- Misses 42719 42723 +4
+ Partials 5240 5238 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const currentHash = window.location.hash.split('?')[0]; | ||
| const isSavedSearchUrl = currentHash.includes('#/view/'); | ||
| const hasUrlParams = window.location.hash.includes('?'); | ||
| const shouldSkipFilters = isSavedSearchUrl && !hasUrlParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get why it should check hasUrlParams here, could you elaborate please?
| syncKeys.push('appFilters'); | ||
| } | ||
|
|
||
| const initialStateFromURL: QueryState = osdUrlStateStorage.get('_q') ?? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe unrelated to this PR: the name initialStateFromURL is a bit misleading, because it's not just url state, it also reads in-memory state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe unrelated to this PR, it seems in connectStorageToQueryState, we're doing two-way state syncing memory state <-> url sate, do you know the intention?
Description
When user click on saved search on Assets page, it will navigate the user to Discover page and load the saved search. However, if user had visited Discover before in same session, Discover will automatcally append the filter state from the previous visit.
This PR allow skip loading previous filter, when user visit Discover page with URL contains saved search without params.
Issues Resolved
fixes #10811
Screenshot
Before:
Screen.Recording.2025-11-12.at.12.18.05.PM.mp4
After:
Screen.Recording.2025-11-12.at.6.21.57.PM.mp4
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration